Fix wallet name generation to use explicit public descriptor checksum by AmosOO7 · Pull Request #482 · bitcoindevkit/bdk_wallet

AmosOO7 · 2026-05-11T11:16:53Z

Summary

Update wallet_name_from_descriptor to compute checksum directly from parsed public descriptor strings
Remove reliance on to_string() implicitly containing #checksum
Clarify in docs that wallet names are defined by public descriptor checksums
Add regression coverage for equivalent xpub/xprv descriptors producing the same wallet name

Testing

cargo test test_wallet_name_from_descriptor_public_key_check

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran just p before pushing

Closes #481

codecov · 2026-05-11T11:20:02Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.91%. Comparing base (6dca318) to head (21a0d4d).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #482      +/-   ##
==========================================
+ Coverage   80.55%   80.91%   +0.35%     
==========================================
  Files          24       24              
  Lines        5472     5469       -3     
  Branches      243      244       +1     
==========================================
+ Hits         4408     4425      +17     
+ Misses        987      967      -20     
  Partials       77       77

Flag	Coverage Δ
rust	`80.91% <100.00%> (+0.35%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

busayo-OD · 2026-05-11T18:16:59Z

-        .to_string();
+    let (descriptor, keymap) = descriptor.into_wallet_descriptor(secp, network_kind)?;
+    if !keymap.is_empty() {
+        return Err(DescriptorError::Miniscript(miniscript::Error::Unexpected(


Since there are already dedicated variants for validation rules like this (HardenedDerivationXpub and ExternalAndInternalAreTheSame), would it make sense to add one here so callers can match on this condition without inspecting the message string?

Thanks, after the comments from @ValuedMammal in #481, this will be removed entirely.

busayo-OD · 2026-05-11T18:17:04Z

+        assert!(result.is_ok());
+
+        // Test with empty descriptor
+        let public_empty = "";


This test returns early at parse_descriptor before keymap.is_empty() is ever checked. It's testing parse error propagation, not the new check. Maybe clarify the comment to reflect that?

Thanks, This will be removed entirely.

aagbotemi

Well done working on this @AmosOO7. The docs for wallet_name_from_descriptor look slightly stale.

aagbotemi · 2026-05-11T19:27:06Z

+            Err(DescriptorError::Miniscript(Unexpected(..)))
+        ));
+        // Test with change descriptor containing public keys (should be ok)
+        let public_change_without_private = "wpkh([9a6a2580/84'/1'/0']tpubDDnGNapGEY6AZAdQbfRJgMg9fvz8pUBrLwvyvUqEgcUfgzM6zc2eVK4vY9x9L5FJWdX8WumXuLEDV5zDZnTfbn87vLe9XceCFwTu9so9Kks/1/*)";


This is a duplicate of public_change above, as they have same descriptor string. One can be removed.

Nice catch, thanks

aagbotemi · 2026-05-11T19:50:44Z

@@ -2797,16 +2797,23 @@ where
    T: IntoWalletDescriptor,
 {
    // TODO: check descriptors contains only public keys


This can be removed now that the check is implemented

Thanks, nice catch

…or checksums - Update `wallet_name_from_descriptor` to compute descriptor checksum explicitly - Remove reliance on `to_string()` implicitly containing `#checksum` - Clarify that wallet name is defined by public descriptor checksums - Add regression test for equivalent xpub/xprv wallet names

ValuedMammal · 2026-05-15T13:14:30Z

In ab1d73b

For clarity you should have somewhere in the test the expected wallet name, for example

assert_eq!(public_result.as_ref().unwrap(), "vn4aqs37");

AmosOO7 · 2026-05-15T13:25:51Z

In ab1d73b

For clarity you should have somewhere in the test the expected wallet name, for example
assert_eq!(public_result.as_ref().unwrap(), "vn4aqs37");

Thank you for that, I will update that now.

Extract public_result.unwrap() into a variable to avoid consuming it twice, enabling comparison between checksums in test

ValuedMammal · 2026-05-15T13:31:44Z

Since we're touching this part of the code we should make the documentation super clear so users know what to expect

Deterministic calculation
Checksum of the parsed public descriptor
If present, the change descriptor checksum is appended
Accepts private descriptors, and produces the same name as the public form
Errors if descriptor parsing fails, or computing the checksum failed

ValuedMammal · 2026-05-15T13:37:15Z

We should update the PR description as well once you've settled on a direction. If you're using links in the PR description, they're not showing up well 🧐

AmosOO7 · 2026-05-15T14:04:23Z

Since we're touching this part of the code we should make the documentation super clear so users know what to expect

Deterministic calculation

Checksum of the parsed public descriptor

If present, the change descriptor checksum is appended

Accepts private descriptors, and produces the same name as the public form

Errors if descriptor parsing fails, or computing the checksum failed

Will push an update for this.

…n clear to understand

AmosOO7 requested a review from ValuedMammal as a code owner May 11, 2026 11:16

github-project-automation Bot added this to BDK Wallet May 11, 2026

AmosOO7 requested a review from oleonardolima as a code owner May 11, 2026 11:16

busayo-OD reviewed May 11, 2026

View reviewed changes

aagbotemi reviewed May 11, 2026

View reviewed changes

ValuedMammal mentioned this pull request May 13, 2026

Clarify wallet_name_from_descriptor behavior and compute checksum explicitly #481

Open

AmosOO7 force-pushed the add_check_for_descriptors-wallet_name_from_descriptor branch 4 times, most recently from 3a7cd0d to 11fb999 Compare May 15, 2026 11:36

AmosOO7 force-pushed the add_check_for_descriptors-wallet_name_from_descriptor branch from 11fb999 to ab1d73b Compare May 15, 2026 11:54

ValuedMammal assigned AmosOO7 May 15, 2026

ValuedMammal moved this to In Progress in BDK Wallet May 15, 2026

ValuedMammal added this to the Wallet 3.1.0 milestone May 15, 2026

Add:

e292716

Extract public_result.unwrap() into a variable to avoid consuming it twice, enabling comparison between checksums in test

AmosOO7 changed the title ~~Add public-key-only validation for wallet_name_from_descriptor~~ Fix wallet name generation to use explicit public descriptor checksum May 15, 2026

Update documentation to make the wallet_name_from_descriptorfunctio…

21a0d4d

…n clear to understand

Conversation

AmosOO7 commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Checklists

All Submissions:

Uh oh!

codecov Bot commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aagbotemi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ValuedMammal commented May 15, 2026

Uh oh!

AmosOO7 commented May 15, 2026

Uh oh!

ValuedMammal commented May 15, 2026

Uh oh!

ValuedMammal commented May 15, 2026

Uh oh!

AmosOO7 commented May 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AmosOO7 commented May 11, 2026 •

edited

Loading

codecov Bot commented May 11, 2026 •

edited

Loading